Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support scss absolute path resolution for url() #7937

Merged
merged 2 commits into from
Jun 7, 2020
Merged

Support scss absolute path resolution for url() #7937

merged 2 commits into from
Jun 7, 2020

Conversation

atlanteh
Copy link
Contributor

@atlanteh atlanteh commented Nov 6, 2019

Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (#7023)
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png

I tested this on my app which has an images directory and after upgrading to CRA 3.2.0 (from 2.1.2) it failed. After reading resolve-url-loader I added this option and changed the path to absolute (/assets/*.png) and everything started working again

@ulrichb
Copy link

ulrichb commented Nov 6, 2019

... build error (Cannot find module '@babel/helper-create-regexp-features-plugin') seems to be unrelated with the change in this PR.

@atlanteh
Copy link
Contributor Author

atlanteh commented Nov 6, 2019

Yes, it's not related. Build is broken for the last 24 hours

@atlanteh
Copy link
Contributor Author

@skrzepij thanks for approving the PR!
When can we expect this to land?

@atlanteh
Copy link
Contributor Author

any update on this? We can't run CI ever since this issue occurred as we need this fix in order for CI to work properly

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 1, 2019

@atlanteh Can you please rebase and push up again - the CI should work now.

@atlanteh
Copy link
Contributor Author

atlanteh commented Dec 1, 2019

@mrmckeb All passed :)

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 1, 2019

I'm OK with this change, @ianschmitz or @iansu - any thoughts?

@atlanteh
Copy link
Contributor Author

atlanteh commented Dec 5, 2019

@mrmckeb @ianschmitz @iansu
If there are no objection, can we proceed with this PR?

@atlanteh
Copy link
Contributor Author

Any update?

@atlanteh
Copy link
Contributor Author

??

@atlanteh
Copy link
Contributor Author

atlanteh commented Jan 7, 2020

Can this PR be merged please?

@atlanteh
Copy link
Contributor Author

@skrzepij @mrmckeb @ianschmitz @iansu
Can you please reply? Why is there a delay in this? This PR was created over two months ago. It's a basic 3 words fixup.
Please let's get this merged

@mrmckeb mrmckeb added this to the 3.4 milestone Jan 13, 2020
@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 13, 2020

I've tagged this for 3.4 so someone should review and merge it before that release. Sorry for the delay, a lot of the team are only returning after Christmas break.

@atlanteh
Copy link
Contributor Author

Thanks!

@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@atlanteh
Copy link
Contributor Author

Can you guys make sure this gets into next release? We really need this fix, and we've been working with this fix locally for 3 months now and it works great

@atlanteh
Copy link
Contributor Author

Why is this moved to milestone 3.6? This is a very small change. This PR is waiting for almost 6 months now. Please merge it 🙏

@arkn98
Copy link

arkn98 commented Apr 29, 2020

+1 Please get this merged.

@arkn98 arkn98 mentioned this pull request May 29, 2020
Closed
@arwagner
Copy link

arwagner commented Jun 5, 2020

Need this fix merged ASAP, please.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 5, 2020

I'm really sorry this one slipped through. We had a quick chat and we think it's good to go, but we'd like to see a quick test added if that's OK @atlanteh?

@atlanteh
Support scss absolute path resolution for url()
2dd16ab 
Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (#7023).
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png
@atlanteh
test: Add global scss assets test
eda3a66
@atlanteh
Copy link
Contributor Author

atlanteh commented Jun 6, 2020

@mrmckeb I added a test, though I see master is currently red, so my tests will fail as well.
I pretty much copy-pasted the bootstrap test and changed what is required. Both these tests cannot run locally due to node-sass is not installed exception, but I see that they pass in the behavior pipelines

mrmckeb
mrmckeb reviewed Jun 7, 2020
View reviewed changes
Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atlanteh The SVG seems odd to me and isn't loading - can you use the logo from our templates?

Or just use a minimal SVG that's just a box? Otherwise I this looks good.

@atlanteh
Copy link
Contributor Author

atlanteh commented Jun 7, 2020 •
edited
Loading

@mrmckeb This is the logo from the templates.. What do you mean by "It isn't loading"? Are you running the tests locally and you can see the results? If so, how?

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 7, 2020

Sorry @atlanteh, I don't know what I did - it's fine, you're right. Hope you're having a great weekend, I'll get this merged in very shortly.

@mrmckeb mrmckeb merged commit fa648da into facebook:master Jun 7, 2020
victor-travelperk pushed a commit to travelperk/create-react-app that referenced this pull request Jun 8, 2020
@atlanteh @victor-travelperk
Add support for absolute paths when using url() in CSS (facebook#7937)
8fa4e28 
* Support scss absolute path resolution for url()

Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (facebook#7023).
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png

* test: Add global scss assets test
@atlanteh atlanteh deleted the patch-1 branch June 9, 2020 07:39
@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
@iansu iansu modified the milestones: 4.1, 4.0 Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ianschmitz ianschmitz ianschmitz approved these changes

@mrmckeb mrmckeb mrmckeb approved these changes

@skrzepij skrzepij skrzepij approved these changes

@iansu iansu Awaiting requested review from iansu

@petetnt petetnt Awaiting requested review from petetnt

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
9 participants
@atlanteh @ulrichb @mrmckeb @arkn98 @arwagner @skrzepij @ianschmitz @iansu @facebook-github-bot